fix: Always accept `{Expr,Series}.is_in(other: Iterable)` #3207

dangotbanned · 2025-10-14T12:34:40Z

Will close #3195

What type of PR is this? (check all applicable)

Related issues

Checklist

Code follows style guide (ruff)
Tests added
Documented the changes

If you have comments or can explain your changes, please do so below

I'm expecting most of the diff to be from this commit:

test: Repurpose Series.from_iterable constructors

The fixing per-backend should only be a couple lines or handling it in nw.Expr

Get tests passing
Update annotations
Do another pass on iterable_to_sequence
Get a decision on Expr.is_in(other: Series)

Will close #3195

Using 4/16 constructors is enough to demonstrate the bug

Preferring to use thrid-party conversions whenever possible

tests/expr_and_series/is_in_test.py

#3207 (comment)

dangotbanned · 2025-10-14T17:21:17Z

narwhals/_utils.py

+class _CanTo_List(Protocol):  # noqa: N801
+    def to_list(self, *args: Any, **kwds: Any) -> list[Any]: ...
+
+
+class _CanToList(Protocol):
+    def tolist(self, *args: Any, **kwds: Any) -> list[Any]: ...


FWIW I hate this as well 😂

Maybe a better idea would be to ...

Rename _CanTo_List -> ToList, and move to _translate.py alongside:

narwhals/narwhals/_translate.py

Lines 124 to 125 in f05011c

class ToDict(Protocol[ToDictDT_co]):

def to_dict(self, *args: Any, **kwds: Any) -> ToDictDT_co: ...

Move these as well, but rename to reflect they their naming originates from numpy and pyarrow (respectively):

narwhals/narwhals/_utils.py

Lines 2131 to 2132 in 0c66432

class _CanToList(Protocol):

def tolist(self, *args: Any, **kwds: Any) -> list[Any]: ...

narwhals/narwhals/_utils.py

Lines 2135 to 2136 in 0c66432

class _CanTo_PyList(Protocol): # noqa: N801

def to_pylist(self, *args: Any, **kwds: Any) -> list[Any]: ...

The names of the guards can still stay the same, since their implementations will (after updating the protocol names) the link between origin, protocol, method name:

narwhals/narwhals/_utils.py

Lines 2139 to 2144 in 0c66432

def can_to_list(obj: Any) -> TypeIs[_CanTo_List]:

return (

is_narwhals_series(obj)

or is_polars_series(obj)

or _hasattr_static(obj, "to_list")

)

narwhals/narwhals/_utils.py

Lines 2147 to 2148 in 0c66432

def can_tolist(obj: Any) -> TypeIs[_CanToList]:

return is_numpy_array_1d(obj) or _hasattr_static(obj, "tolist")

narwhals/narwhals/_utils.py

Lines 2151 to 2154 in 0c66432

def can_to_pylist(obj: Any) -> TypeIs[_CanTo_PyList]:

return (

(pa := get_pyarrow()) and isinstance(obj, (pa.Array, pa.ChunkedArray))

) or _hasattr_static(obj, "to_pylist")

I updated my comment (#3207 (comment)) - I would argue that native series are not ok, while numpy 1d arrays are.

My thought process for this is that if someone is doing something along the lines of:

import narwhals as nw import polars as pl def agnostic_func(frame: IntoDataFrameT) -> IntoDataFrameT: other = pl.Series([1, 2, 3]) # <- notice how this is a native series!!! return nw.from_native(frame).filter(nw.col("x").is_in(other)).to_native()

then the function is clearly not agnostic and polars would be required in this case.

A different case would be if a narwhals series with a different backend is provided. This could mean that the function is agnostic but a user is "mixin" backends:

def is_left_in_right(left_series: IntoSeriesT, right_series: IntoSeriesT) -> IntoSeriesT: left_nw = nw.from_native(left_series, series_only=True) right_nw = nw.from_native(right_series, series_only=True) return left_nw.is_in(right_nw).to_native() # but now it a user to mix it up, not the library itself is_left_in_right(pl.Series([1,2,3]), pd.Series([0, 1]))

This is the case I suggested to yell at the user with a warning.

From our side, I think it would greatly simplify (read as, get rid of) most of the protocols here, the type guards as well as iterable_to_sequence function.

dangotbanned · 2025-10-14T17:22:43Z

tests/conftest.py

+_into_iter: Callable[[int], Iterator[IntoIterable]] = _into_iter_selector()
+"""`into_iter` fixtures use the suffix `_<n>` to denote the maximum number of constructors.
+
+Anything greater than **10** may return less depending on available dependencies.
+"""
+
+
+@pytest.fixture(params=_into_iter(16), scope="session", ids=_ids_into_iter)
+def into_iter_16(request: pytest.FixtureRequest) -> IntoIterable:
+    function: IntoIterable = request.param
+    return function


Kinda outdated now since (633a06d)

narwhals/_utils.py

dangotbanned · 2025-10-14T18:33:45Z

narwhals/expr.py

        if isinstance(other, Iterable) and not isinstance(other, (str, bytes)):
+            other = other.to_native() if is_series(other) else iterable_to_sequence(other)
            return self._with_elementwise(
-                lambda plx: self._to_compliant_expr(plx).is_in(
-                    to_native(other, pass_through=True)
-                )
+                lambda plx: self._to_compliant_expr(plx).is_in(other)


I've updated this but it is still wrong.
Didn't notice the issue until I started trying to add typing to the compliant-level

narwhals/narwhals/_compliant/column.py

Line 173 in 849326f

def is_in(self, other: Any) -> Self: ...

I don't understand why we were allowing any kind of Native* to be passed to every backend? 🤔

Gonna add another test for at least the more common case of a Series from the wrong backend

Updated

test: Add test_expr_is_in_series_wrong_backend

I'm not sure of a safe way to keep the same behavior (if it is desired).
Since we don't know the backend at this stage, the options I see are:

Unconditionally convert to list | tuple

Raise elsewhere when a Series is passed to a lazy backend

Disallow Expr.is_in(other: Series)

Do nothing

@MarcoGorelli @FBruzzesi
do you guys have any preference on a path forward here?

This case is a bit different to some of the other places we'd reject nw.Series for lazy backends - since the length isn't an issue.
But the safer option of converting all nw.Series is gonna be less performant than the currently unsafe version (which only works on a matching eager implementation)

Everything seems like a tradeoff to me 😔

I've moved out of draft for visibility (#3207 (comment))

Hey @dangotbanned I will try to take a look during the weekend, but I am not sure I can manage.

My thoughts from the context in the messages here (I didn't open the code changes just yet):

Realistically I don't think it's too common to pass one series from a different backend but we should not exclude such possibility

My opinion would be to:

Use the native series if isinstance(other, Series) and expr._implementation == other._implementation)

Otherwise convert it to a list (we can do that with native methods thankfully), but warn that such conversion is happening with a UserWarning. This case includes a series passed to a lazy backend

Update: Regarding native series, then I would prefer to raise an exception in such case

See #3207 (comment)

…able

dangotbanned added 2 commits October 14, 2025 12:31

test: Repurpose Series.from_iterable constructors

234a091

Will close #3195

test: Add test_expr_is_in_iterable

26ae2eb

Using 4/16 constructors is enough to demonstrate the bug

dangotbanned changed the title ~~fix(DRAFT): Always accept `Expr.is_in(other: Iterable)~~ fix(DRAFT): Always accept Expr.is_in(other: Iterable) Oct 14, 2025

dangotbanned added fix duckdb Issue is related to duckdb backend polars Issue is related to polars backend sqlframe Issue is related to sqlframe backend labels Oct 14, 2025

dangotbanned added 5 commits October 14, 2025 13:37

test: Add test_ser_is_in_iterable

ce8e591

chore: Add _utils.iterable_to_sequence

98cc8bd

Preferring to use thrid-party conversions whenever possible

fix: Series.is_in

2cd832c

fix: Expr.is_in

38d6d5d

coverage?

633a06d

dangotbanned commented Oct 14, 2025

View reviewed changes

tests/expr_and_series/is_in_test.py Show resolved Hide resolved

dangotbanned changed the title ~~fix(DRAFT): Always accept Expr.is_in(other: Iterable)~~ fix(DRAFT): Always accept {Expr,Series}.is_in(other: Iterable) Oct 14, 2025

test: Track down the xfail

0c66432

#3207 (comment)

dangotbanned commented Oct 14, 2025

View reviewed changes

narwhals/_utils.py Outdated Show resolved Hide resolved

dangotbanned commented Oct 14, 2025

View reviewed changes

dangotbanned added 4 commits October 14, 2025 18:34

test: Add test_expr_is_in_series_wrong_backend

a6921ea

See #3207 (comment)

chore(typing): Update narwhals-level typing

5409c21

Update narwhals/_utils.py

b08ffe0

chore(typing): Update compliant-level typing

ffc848c

See #3207 (comment)

dangotbanned changed the title ~~fix(DRAFT): Always accept {Expr,Series}.is_in(other: Iterable)~~ fix: Always accept {Expr,Series}.is_in(other: Iterable) Oct 14, 2025

dangotbanned added help wanted Extra attention is needed and removed duckdb Issue is related to duckdb backend polars Issue is related to polars backend sqlframe Issue is related to sqlframe backend labels Oct 14, 2025

Merge remote-tracking branch 'upstream/main' into fix/expr-is-in-iter…

e318bdf

…able

dangotbanned marked this pull request as ready for review October 18, 2025 11:29

Merge branch 'main' into fix/expr-is-in-iterable

e9eb08e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Always accept `{Expr,Series}.is_in(other: Iterable)` #3207

fix: Always accept `{Expr,Series}.is_in(other: Iterable)` #3207

Uh oh!

dangotbanned commented Oct 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

dangotbanned Oct 14, 2025

Uh oh!

dangotbanned Oct 15, 2025 •

edited

Loading

Uh oh!

FBruzzesi Oct 18, 2025 •

edited

Loading

Uh oh!

dangotbanned Oct 14, 2025

Uh oh!

Uh oh!

dangotbanned Oct 14, 2025 •

edited

Loading

Uh oh!

dangotbanned Oct 14, 2025 •

edited

Loading

Uh oh!

dangotbanned Oct 18, 2025

Uh oh!

FBruzzesi Oct 18, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	class ToDict(Protocol[ToDictDT_co]):
	def to_dict(self, args: Any, *kwds: Any) -> ToDictDT_co: ...

	class _CanToList(Protocol):
	def tolist(self, args: Any, *kwds: Any) -> list[Any]: ...

	class _CanTo_PyList(Protocol): # noqa: N801
	def to_pylist(self, args: Any, *kwds: Any) -> list[Any]: ...

	def can_to_list(obj: Any) -> TypeIs[_CanTo_List]:
	return (
	is_narwhals_series(obj)
	or is_polars_series(obj)
	or _hasattr_static(obj, "to_list")
	)

	def can_tolist(obj: Any) -> TypeIs[_CanToList]:
	return is_numpy_array_1d(obj) or _hasattr_static(obj, "tolist")

	def can_to_pylist(obj: Any) -> TypeIs[_CanTo_PyList]:
	return (
	(pa := get_pyarrow()) and isinstance(obj, (pa.Array, pa.ChunkedArray))
	) or _hasattr_static(obj, "to_pylist")

fix: Always accept {Expr,Series}.is_in(other: Iterable) #3207

Are you sure you want to change the base?

fix: Always accept {Expr,Series}.is_in(other: Iterable) #3207

Uh oh!

Conversation

dangotbanned commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this? (check all applicable)

Related issues

Checklist

If you have comments or can explain your changes, please do so below

Uh oh!

Uh oh!

dangotbanned Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

dangotbanned Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBruzzesi Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangotbanned Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dangotbanned Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Updated

Uh oh!

dangotbanned Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangotbanned Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

FBruzzesi Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: Always accept `{Expr,Series}.is_in(other: Iterable)` #3207

fix: Always accept `{Expr,Series}.is_in(other: Iterable)` #3207

dangotbanned commented Oct 14, 2025 •

edited

Loading

dangotbanned Oct 15, 2025 •

edited

Loading

FBruzzesi Oct 18, 2025 •

edited

Loading

dangotbanned Oct 14, 2025 •

edited

Loading

dangotbanned Oct 14, 2025 •

edited

Loading

FBruzzesi Oct 18, 2025 •

edited

Loading